Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

adding spin to path #193

Closed
wants to merge 4 commits into from
Closed

adding spin to path #193

wants to merge 4 commits into from

Conversation

macolso
Copy link
Contributor

@macolso macolso commented Nov 2, 2022

Signed-off-by: MacKenzie Olson 48335676+macolso@users.noreply.github.com

Content must go through a pre-merge checklist.

Pre-Merge Content Checklist

This documentation has been checked to ensure that:

  • The title, template, and date` are all set
  • File does not use CRLF, but uses plain LF (hint: use cat -ve <filename> | grep '^M' | wc -l and expect 0 as a result)
  • Has passed bart check
  • Has been manually tested by running in Spin/Bartholomew (hint: use PREVIEW_MODE=1 and run npm run styles to update styling)
  • Headings are using Title Case
  • Code blocks have the programming language set to properly highlight syntax and the proper copy directive
  • Have tested with npm run test and resolved all errors

@macolso
adding spin to path
8832a6d 
Signed-off-by: MacKenzie Olson <48335676+macolso@users.noreply.github.com>
@macolso
Copy link
Contributor Author

macolso commented Nov 2, 2022

Added to PATH. @mikkelhegn would be good to get your review given 822

@mikkelhegn
Copy link
Member

I'm a little concerned with this as it might break a flow which is deliberately very hard to break. Not sure if everyone can run the sudo command. Can we somehow make it optional by adding it in a quote section or but it in the "Learn more".

I think the real way to solve this is through spinframework/spin#641 and then make the quickstart use those.

@mikkelhegn
Copy link
Member

Let me know if I'm too much of a chicken :-)

@macolso
Moved the path recommendation intro the drop down
bb9a8d2 
Signed-off-by: MacKenzie Olson <48335676+macolso@users.noreply.github.com>
@macolso
Copy link
Contributor Author

macolso commented Nov 3, 2022

I'm aligned with that thought process - we may lose some users who don't have the ability to run sudo if we put PATH in the mainstream directions.

I really like how Nomad's UI on their install options . We should do something like once spinframework/spin#641 is completed

@mikkelhegn
Copy link
Member

@tpmccallum ☝🏻

@itowlson
Copy link
Contributor

itowlson commented Nov 7, 2022

rustup seems to be able to add the Rust tooling to PATH without sudo, by installing into ~/.cargo, then adding . "$HOME/.cargo/env" to .bashrc (which in turn adds ~/.cargo/bin to PATH). Makes the installer a bit more complicated of course...

@macolso
Update quickstart.md
be14d72 
Signed-off-by: MacKenzie Olson <48335676+macolso@users.noreply.github.com>
karthik2804
karthik2804 requested changes Nov 7, 2022
View reviewed changes
Copy link
Contributor

@karthik2804 karthik2804 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handlebars does not support multiline strings. Therefore the line breaks must be represented using \n

@macolso @karthik2804
Update content/cloud/quickstart.md
6fd88b8 
Co-authored-by: Karthik Ganeshram <karthik.ganeshram@fermyon.com>
Signed-off-by: MacKenzie Olson <48335676+macolso@users.noreply.github.com>
@macolso
Copy link
Contributor Author

macolso commented Nov 7, 2022

Going to hold off merging until we have a complete installation story

@macolso macolso closed this Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikkelhegn mikkelhegn mikkelhegn left review comments

@karthik2804 karthik2804 karthik2804 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@macolso @mikkelhegn @itowlson @karthik2804